Skip to content

Enable AOF #255

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Dec 9, 2019
Merged

Enable AOF #255

merged 5 commits into from
Dec 9, 2019

Conversation

lantiga
Copy link
Contributor

@lantiga lantiga commented Dec 6, 2019

With #157 having been merged, this PR enables AOF and runs the test suite with both RDB and AOF persistence.

TODO: activate AOF tests in CI

@lantiga
Copy link
Contributor Author

lantiga commented Dec 6, 2019

Odd, running tests with useSlaves=True passed to the Env constructor leads to the test runner to hang indefinitely after the first test passes.

@rafie @MeirShpilraien does this ring a bell?

@MeirShpilraien
Copy link

@lantiga I see that you are getting the env on the test function and then create it (again) at the test itself. This actually cause 2 env's to be created at your test and then undefined behavior (when I say undefined I mean that I did not check what might happened in this case).
The way that I meant it to be use is either to get the environment at the test as argument and this way the environment will be create as requested when running the test, or to created the environment inside the test and then created it as the test need it (but not combine them both).

This way for example you can create a test that run both on single shard and with slave by just writing the test generically and then run it twice, one with the --use-slave argument and one without.

On the other hand you might want your test to run only with slave and then you need to not receive the env as an argument and just create it inside the test the way you want.

Hope it was clear enough, let me know if it helped.

@lantiga
Copy link
Contributor Author

lantiga commented Dec 9, 2019

@MeirShpilraien thank you for your answer, that clears it. When I saw the env as an argument, I thought there would be a way to augment the incoming environment with programmatically set arguments.

At this point I would rather rely on command-line arguments, so we can execute the same tests in different situations.

@lantiga
Copy link
Contributor Author

lantiga commented Dec 9, 2019

Looking for a review, then ready to merge

python3 -m RLTest $(TEST_ARGS) --test basic_tests.py --module $(INSTALL_DIR)/redisai.so --use-slaves ;\
python3 -m RLTest $(TEST_ARGS) --test double-panda.py --module $(INSTALL_DIR)/redisai.so
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was failing on macOS, so this line actually waits for the macOS CI to be enabled. If we're convinced that everything is in order there, we can remove the rest of the test artifacts. Otherwise, it may be worthwhile to just comment it out until CI is ready.

rafie
rafie previously approved these changes Dec 9, 2019
@lantiga
Copy link
Contributor Author

lantiga commented Dec 9, 2019

@K-Jo FYI I updated the pack version here

@lantiga lantiga merged commit 8961b31 into master Dec 9, 2019
lantiga added a commit that referenced this pull request May 6, 2020
 
Enable AOF (#255)

* Enable AOF

* Enable AOF in tests via env conf

* Fix use of Env in tests. Run test suite multiple times with different env configurations.

* Comment out panda test

* Update pack version and license in RAMP file
@gkorland gkorland deleted the enable_aof branch October 6, 2020 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants